-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adopt Codable #127
Adopt Codable #127
Conversation
Surprisingly, it turned out to be approximately the same amount of code after adopting Codable and JSONDecoder but still worth it considering improved type safety and cleaner code. Using structs and the same property names on the client and server with default encode/decode implementations would probably have reduced the amount of code a bit more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I agree that the improved type safety makes it worth landing this refactoring – thanks for taking it on. I wouldn’t worry too much about the increase in line count. If not for accommodating platform naming conventions, this library would be pretty much out of a job. 😉
code = try container.decodeIfPresent(String.self, forKey: .code)?.uppercased() | ||
if let identifier = try container.decodeIfPresent(String.self, forKey: .wikidataItemIdentifier) { | ||
assert(identifier.hasPrefix("Q")) | ||
wikidataItemIdentifier = identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we’re optimistically decoding the QualifyingPlacemark properties in the superclass’s decoder. That’s alright, since GeocodedPlacemark and QualifyingPlacemark don’t have any identically named properties with different implementations. However, it probably points to a need to eventually turn Placemark itself into a protocol.
Fixes #126 adopt Codable along with JSONDecoder
@1ec5 @JThramer @bsudekum 👀